feature: add the balancer.recreate_request function, which allows user to recreate request buffer in balancer phase.#302
Merged
doujiang24 merged 5 commits intomasterfrom Jul 30, 2020
Conversation
Member
|
@dndx just a reminder, the CI failed. |
dndx
force-pushed
the
proxy_recreate_request
branch
from
ef551a4 to
20204f1
Compare
Member
Author
|
@doujiang24 Yes the test plan was wrong. I have fixed the number. |
dndx
force-pushed
the
proxy_recreate_request
branch
from
20204f1 to
5a27cf2
Compare
dndx
added a commit
to Kong/kong-build-tools
that referenced
this pull request
Jun 30, 2020
…ocao These will be removed once openresty/lua-nginx-module#1734 and openresty/lua-resty-core#302 are merged
dndx
added a commit
to Kong/kong-build-tools
that referenced
this pull request
Jul 1, 2020
…ocao These will be removed once openresty/lua-nginx-module#1734 and openresty/lua-resty-core#302 are merged
Member
Author
|
Ping @doujiang24. Could you give another look at this? Thanks! |
thibaultcha
requested changes
Jul 16, 2020
lib/ngx/balancer.lua
Outdated
| long read_timeout, char **err); | ||
|
|
||
| int | ||
| ngx_http_lua_ffi_balancer_recreate_request(ngx_http_request_t *r, |
Member
There was a problem hiding this comment.
style: FFI prototype declarations should have the return type on the same line, like in the C module
lib/ngx/balancer.md
Outdated
| [Back to TOC](#table-of-contents) | ||
|
|
||
| recreate_request | ||
| ------------ |
Member
There was a problem hiding this comment.
style: this line should underline the whole method name
lib/ngx/balancer.md
Outdated
|
|
||
| **context:** *balancer_by_lua** | ||
|
|
||
| Regenerates the request buffer for sending to the upstream server. This is useful, for example |
Member
There was a problem hiding this comment.
Maybe better to stay consistent with wording? i.e. use "recreate" instead of "regenerate" which is the terminology used everywhere else when referring to this API.
lib/ngx/balancer.md
Outdated
|
|
||
| Normally this does not work because the request buffer is created once during upstream module | ||
| initialization and won't be regenerated for subsequent retries. However you can use | ||
| `proxy_set_header My-Header $my_header` and sets the `ngx.var.my_header` variable inside the |
lib/ngx/balancer.md
Outdated
| Normally this does not work because the request buffer is created once during upstream module | ||
| initialization and won't be regenerated for subsequent retries. However you can use | ||
| `proxy_set_header My-Header $my_header` and sets the `ngx.var.my_header` variable inside the | ||
| balancer phase. Calling this function after that will cause the request buffer to be re-generated |
Member
There was a problem hiding this comment.
"Calling this function after that" can be confusing for the reader. "Calling balancer.recreate_request() after updating a header field will cause..."
lib/ngx/balancer.md
Outdated
| and the `My-Header` header will thus contain the new value. | ||
|
|
||
| **Warning:** because the request buffer has to be recreated and such allocation occurs on the | ||
| request memory pool, the old buffer has to be thrown away and only be freed after the request |
Member
There was a problem hiding this comment.
and will only be freed when...
to recreate request buffer in balancer phase. This allows certain request parameters, such as headers (including `Host` header) to be modified between balancer retries.
dndx
force-pushed
the
proxy_recreate_request
branch
from
5a27cf2 to
1e749c8
Compare
Member
Author
|
Thanks for the reviews @thibaultcha, I fixed the styling and ran the document through with |
doujiang24
approved these changes
Jul 18, 2020
syzh
approved these changes
Jul 19, 2020
agentzh
approved these changes
Jul 20, 2020
agentzh
requested changes
Jul 20, 2020
lib/ngx/balancer.md
Outdated
| to this function should be made **only** if you know the request buffer must be regenerated, | ||
| instead of unconditionally in each balancer retries. | ||
|
|
||
| This function was first added in the `0.1.19` version of this library. |
Member
There was a problem hiding this comment.
This should be changed to 0.1.20.
agentzh
requested changes
Jul 20, 2020
t/balancer.t
Outdated
| print("here") | ||
| local b = require "ngx.balancer" | ||
|
|
||
| if ngx.ctx.balancer_ran then |
Member
There was a problem hiding this comment.
I think balancer_run is more grammatically correct.
doujiang24
pushed a commit
that referenced
this pull request
Jul 30, 2020
…ser to recreate request buffer in balancer phase. (#302) This allows certain request parameters, such as headers (including `Host` header) to be modified between balancer retries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-resty-core project.
Feature was originally developed by @locao inside Kong/lua-kong-nginx-module#13. With slight modifications from @dndx.
Related PR: openresty/lua-nginx-module#1734.